chore(sequencer)!: exclusively use Borsh encoding for stored data (ENG-768) #1492
Conversation
Fraser999
requested review from
a team,
joroshiba,
SuperFluffy and
noot
as code owners
github-actions
bot
added
conductor
There was a problem hiding this comment.
High level comment on the approach taken in this design:
At a high level, sequencer is defined by a set of components, each of which provides ways to change and read their respective state. So far this also meant that each component had full control over how the objects written to its state are converted to/from the domain types.
The storage and stored_value modules invert this logic by defining and passing in the on-disk types into the components.
I am ambivalent about this change since on the one hand it potentially allows avoiding code duplication. But on the other hand it breaks with the separation between the components we had so far.
I see that as one of the main problems with the existing approach. It's because we've allowed this flexibility that different components use different serialization for the same domain types. In a lot of cases, there is only the domain type - the component would serialize directly from the domain type into bytes. I'm very keen to remove that flexibility so that all components have to use
This is only temporary until #1436 is completed. When that work is completed, the components will be dealing with the domain types only.
Well, I don't think there's complete separation currently. For example, |
There was a problem hiding this comment.
I have went through about half the changes, but I think my comments apply universally.
- We should provide domain types for all objects that have their own on-disk borsh types (for example,
feehasstorage::Fee, it feels natural to make it adomain::Fee, too). The on-disk types right now are stricter (which is good) than the domain types. - I am ambivalent about many on-disk types being
pub(crate). I prefer if the on-disk types were module private and never crossed the boundary. - A lot of error instances lack context but have a comment above them noting that enough error context is provided. Because skipping context by just escaping with an
?operator is all too common among junior engineers, I'd prefer to require them everywhere rather than to come up with a heuristic as to when to use them and when to skip.
crates/astria-sequencer/src/app/storage/values/storage_version.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer/src/app/storage/values/revision_number.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer/src/bridge/init_bridge_account_action.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Comments:
- the errors for failing to serialize from/to the on-disk type should use
Debug(very rare, very bad if it happens: it's fine to be extra noisy). - on-disk types with a
Displayimpl should should have a unit test so that their display impl matches that of the domain type (to avoid confusion) non-exhaustive list of types where this applies:RollupId,BlockHash. - we should provide an
OndiskTypetrait that encapsulates theFrom/TryFromimpls we have everywhere right now. - we really really should split the tests into separate modules. This is not the first PR where I wish I could just ignore the tests when going through the code. :-/
crates/astria-sequencer/src/grpc/storage/values/sequencer_block_header.rs
Outdated
Show resolved
Hide resolved
|
|
||
| #[expect( | ||
| clippy::default_trait_access, | ||
| reason = "want to avoid explicitly importing `index_map` crate to sequencer crate" |
There was a problem hiding this comment.
A good constructor would be even better! One which took an iterator over RollupTransactions so we don't need to care how it's held internally :)
There was a problem hiding this comment.
Thank you for the heroic task of going through all of sequencer and fixing its state write/read. I fat fingered my previous review but the comments were intended for this approval.
I don't see any blockers. From the previous list my main ask is to get remove all custom display impls in favor of just the derived Debug - we discussed this in the relevant comment chain, but as you noted the Display impls are intended for providing errors on failed state-decoding.
If that happens we are in so deep waters, that I think spewing a Debug log all over the screen is perfectly acceptable.
(side note that didn't make it into the list of the other comment:
- We should extend snapshot tests to all keys - but that's likely covered in your open followup PR.
)
crates/astria-sequencer/src/bridge/storage/values/transaction_id.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer/src/bridge/storage/values/address_bytes.rs
Outdated
Show resolved
Hide resolved
* main: (34 commits) feat(proto): add bundle and optimistic block apis (#1519) feat(sequencer)!: make empty transactions invalid (#1609) chore(sequencer): simplify boolean expressions in `transaction container` (#1595) refactor(cli): merge argument parsing and command execution (#1568) feat(charts): astrotrek chart (#1513) chore(charts): genesis template to support latest changes (#1594) fix(ci): code freeze action fix (#1610) chore(sequencer)!: exclusively use Borsh encoding for stored data (ENG-768) (#1492) ci: code freeze through github actions (#1588) refactor(sequencer): use builder pattern for transaction container tests (#1592) feat(conductor)!: implement chain ID checks (#1482) chore(ci): upgrade audit-check (#1577) feat(sequencer)!: transaction categories on UnsignedTransaction (#1512) fix(charts): sequencer prometheus rules (#1598) chore(all): Migrate all instances of `#[allow]` to `#[expect]` (#1561) chore(charts,sequencer-faucet): asset precision (#1517) chore(docs): endpoints (#1543) fix(docker): use target binary build param as name of image entrypoint (#1573) fix(ci): ibc bridge test timeout (#1587) Feature: Add `graph-node` to charts. (#1489) ...
Summary
This PR primarily changes the encoding format of all data being written to storage to Borsh.
Background
We currently have a variety of different encoding formats, and this can be confusing, sub-optimal (in terms of storage space consumed and serialization/deserialization performance) and potentially problematic as e.g. JSON-encoding leaves a lot of flexibility in how the actual serialized data can look.
This PR is part one of three which aim to improve the performance and quality of the storage component. As such, the APIs of the various
StateReadExtandStateWriteExtextension traits were updated slightly in preparation for the upcoming changes. In broad terms, for getters this meant having ref parameters rather than value ones (even for copyable types like[u8; 32]this is significantly more performant), and for "putters", parameters which are used for DB keys are refs, while the DB value parameters become values, since in the next PR these values will be added to a cache.Changes
storagemodule. This will ultimately contain our own equivalents of thecnidariumtypes, but for now consists only of a collection of submodules for types currently being written to storage. There is a top-level enumStoredValuewhich becomes the only type being written to storage by our own code.astria-coreandastria-merkle, some of these have been provided with constructors namedunchecked_from_parts. This allows the type to be constructed from a trusted source like our own DB, skipping any validation steps which might otherwise be done.StateReadExtandStateWriteExttraits to use the newStoredValue, which internally uses Borsh encoding.[u8; 32]and[u8; 20]arrays, and that has unfortunately made the PR larger than expected.serdetraits have had that removed, since it only existed to support JSON-encoding the type for storing. In one case it was for JSON-encoding to a debug log line, but I replaced that with aDisplayimpl.Testing
StateReadExtandStateWriteExttraits already had almost full coverage. Where coverage was missing, I added round trip tests.Breaking Changelist
Related Issues
Closes #1434.